KAFKA-12233: Align the length passed to FileChannel by FileRecords.writeTo#9970
Conversation
|
Hi, @junrao , PTAL. |
dengziming
changed the title
FileRecords.writeTo set length incorrectlyFileRecords.writeTo set length incorrectly
|
@dengziming Thanks for your patch. Does this patch want to adjust "count" to make sure it is NOT out of bound of file size? If so, it may be redundant as the "count" is maximum number to transfer rather than the "absolute" number to transfer. It seems to me all implementation of |
|
@chia7712 You are right, in fact all implementation of |
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@dengziming : Thanks for the PR. Left a comment below.
|
|
||
| long position = start + offset; | ||
| int count = Math.min(length, oldSize); | ||
| long count = Math.min(length, oldSize - offset); |
There was a problem hiding this comment.
This code is called from SocketServer through RecordsSend. The expectation is that the underlying file channel has enough bytes to return for the requested length. Normally this should be the case. If for some reason this is not true, by bounding the transfer amount by the remaining bytes in the file channel will cause RecordsSend.writeTo() to be called in a loop until the request times out. This may hide a real problem.
Perhaps what we could do is to throw an IllegalStateException if the file channel doesn't have enough bytes left. This will cause the socket channel to be closed and the Selector to log a warning.
There was a problem hiding this comment.
Perhaps what we could do is to throw an IllegalStateException if the file channel doesn't have enough bytes left. This will cause the socket channel to be closed and the Selector to log a warning.
Please update docs for related writeTo methods if we want to apply this behavior. The current code use return value to calculate the length for next write. The return value gets redundant since the number of transferred data is always equal to expected length (it throws exception when there is no enough bytes left).
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@dengziming Thanks for nice #9819 and aligning the count is good to me.
|
@dengziming Could you revise the title? It seems to me the passed length is NOT incorrect but it has room to tweak the value. How about "align the length passed to FileChannel by FileRecords.writeTo"? |
dengziming
changed the title
FileRecords.writeTo set length incorrectlyFileRecords.writeTo
|
@chia7712 Thank you for your suggestions, changed the title. Maybe we can discuss the |
chia7712
changed the title
FileRecords.writeToFileRecords.writeTo
3 participants
More detailed description of your change
See https://issues.apache.org/jira/browse/KAFKA-12233
Summary of testing strategy (including rationale)
unit test
Committer Checklist (excluded from commit message)